Support test_code to add pyright-config comment#98
Support test_code to add pyright-config comment#98F-park wants to merge 3 commits intolaike9m:mainfrom
Conversation
|
I should rewrite the test to make it pass |
|
Could you describe what problems you're trying to solve, and what other ways you have considered (if any)? Thanks. |
class MyClass:
def __init__(self, x: int) -> None:
self.x = x
# TODO: Fix the type hints of `copy` to make it type check
def copy(self):
copied_object = MyClass(x=self.x)
return copied_objectIt can pass the challenge unexpectedly without return_value annoating and you said |
|
Thanks for the PR. Surely there's value in enabling the configs, however it also brings more complexity. At this point, I don't think it's worth adding it just to fix a tiny problem. With that said, in the future it might become more useful, so I'll keep this open and re-evaluate when the time comes. Hope you can understand. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR introduces test code partitioning to separate user-written test code from Pyright configuration directives. A new Changes
Sequence DiagramsequenceDiagram
participant User as Challenge Code
participant CM as ChallengeManager
participant Part as _partition_test_code()
participant PR as Pyright
participant Map as Error Mapper
participant Report as Error Report
User->>CM: run_type_check(test_code)
CM->>Part: _partition_test_code(test_code)
Part-->>CM: (user_code, config_block)
CM->>CM: Merge config + BASIC_CONFIG
CM->>PR: Execute Pyright on augmented code
PR-->>CM: Error diagnostics with line numbers
CM->>Map: Map line numbers to sections
Map->>Map: Identify user code / test code / config lines
Map-->>CM: Mapped errors with source origin
CM->>Report: Categorize errors (config errors non-failing)
Report-->>User: Unified error count & diagnostics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Warning Tools execution failed with the following error: Failed to run tools: Ping-pong health check failed Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @laike9m. * #98 (comment) The following files were modified: * `tests/assets/challenges/basic-foo-pyright-config/question.py` * `tests/conftest.py` * `tests/test_identical.py` * `views/challenge.py` * `views/views.py`
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @docs/Contribute.md:
- Line 45: The sentence fragment should be rewritten so it reads as a complete
instruction: change the line that currently reads about the optional comment
marker and pyright config to something like: "Optionally add a comment `## End
of test code ##`. You may also include pyright configuration options using the
format `# pyright: <config_name>=<value>`." Update the wording around the `##
End of test code ##` marker and the `# pyright: <config_name>=<value>` example
so the two clauses form complete, grammatical sentences.
In @tests/test_identical.py:
- Around line 11-24: get_test_code currently ignores its path arg and always
reads solution_file, so both solution_test and question_test come from the same
file; open and read the provided path parameter instead of solution_file inside
get_test_code (fix the file open call used when building challenge_code and test
extraction), keeping the rest of the function logic the same so solution_test
and question_test are derived from their respective paths.
In @views/challenge.py:
- Around line 261-268: The error count currently includes non-blocking
pyright-config lines and the singular/plural grammar is wrong; filter out lines
starting with "[pyright-config]" into a blocking_errors list and use that to
determine passed (set passed = True when len(blocking_errors) == 0) and to build
the summary message, e.g., append f"\nFound {n} error" vs "errors" based on n
where n = len(blocking_errors), or include both totals like "Found X errors (Y
blocking)"; update references to error_lines and the append call to use the new
counts.
🧹 Nitpick comments (2)
tests/conftest.py (1)
26-29: LGTM!The fixture correctly creates a
ChallengeManagerinstance using the test assets directory.Consider adding
scope="module"orscope="session"if theChallengeManageris stateless and reads from disk, to avoid recreating it for each test function:♻️ Optional optimization
-@pytest.fixture() +@pytest.fixture(scope="module") def mgr(assets_dir: Path): return ChallengeManager(assets_dir / "challenges")views/challenge.py (1)
175-186: Clarify the pyright config override behavior.The comment on line 179 states the goal is to "limit user to modify the config." However, appending
pyright_configafterPYRIGHT_BASIC_CONFIG(line 185) allows challenge authors to override the basic settings, since later pyright inline comments take precedence.If this is intentional (challenge authors can customize, but end-users submitting code cannot), consider updating the comment to clarify this distinction.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
challenges/advanced-forward/question.pydocs/Contribute.mdtests/assets/challenges/basic-foo-pyright-config/question.pytests/conftest.pytests/test_challenge.pytests/test_identical.pytests/test_questions.pyviews/challenge.pyviews/views.py
💤 Files with no reviewable changes (1)
- tests/test_questions.py
🔇 Additional comments (7)
challenges/advanced-forward/question.py (1)
17-19: LGTM!The test scaffolding correctly demonstrates the new pyright-config feature with the
## End of test code ##marker followed by a valid pyright directive.views/views.py (1)
62-62: LGTM!The
partition()approach handles both cases correctly: it returns the substring before the marker when present, or the full string if absent. The newline-wrapped marker"\n## End of test code ##\n"ensures exact matching of the standalone comment line.tests/assets/challenges/basic-foo-pyright-config/question.py (1)
10-11: Verify the test expectation forfoo(1).
foo()is defined without parameters, so bothfoo(1)andfoo(1, 2)should produce type errors. However, only line 11 is marked with# expect-type-error. Is this intentional for testing specific pyright config behavior, or should line 10 also be marked?tests/test_challenge.py (1)
16-22: LGTM!The test correctly verifies the
_partition_test_codebehavior by checking that the returned pyright config ends with the expected configuration line fromPYRIGHT_BASIC_CONFIG.views/challenge.py (3)
14-36: LGTM!The pyright configuration covers appropriate strictness settings for a type-checking challenge platform.
244-251: LGTM!The error line mapping correctly translates absolute line numbers from the merged code back to their respective sections (user code, test code, or pyright-config).
255-259: Verify: Line number adjustment assumes missing expected errors are always in test code.Line 258 unconditionally subtracts
user_code_lines_lenfrom the line number. If anexpect-type-errorcomment could appear in user code (line numbers ≤user_code_lines_len), this would produce incorrect (zero or negative) line numbers in the error message.If
expect-type-erroris only valid in test code, this is fine. Otherwise, consider applying the same section-based mapping used for other errors (lines 246-251).
| - Describe the challenge, make sure people understand what they need to accomplish (i.e. the `TODO:` part) | ||
| - A comment `## End of your code ##`. This is mandatory, just copy and paste it. | ||
| - Several test cases. Add a comment `# expect-type-error` after the lines where type errors should be thrown. | ||
| - (Optional) Add a comment `## End of test code ##`. Several [pyright-config](https://github.com/microsoft/pyright/blob/main/docs/configuration.md#type-check-diagnostics-settings) with the format `# pyright: <config_name>=<value>` |
There was a problem hiding this comment.
Minor grammatical issue in the documentation.
The sentence structure is incomplete. After describing the comment marker, it reads "Several [pyright-config]..." which doesn't form a complete sentence.
📝 Suggested fix
- - (Optional) Add a comment `## End of test code ##`. Several [pyright-config](https://github.com/microsoft/pyright/blob/main/docs/configuration.md#type-check-diagnostics-settings) with the format `# pyright: <config_name>=<value>`
+ - (Optional) Add a comment `## End of test code ##`, followed by [pyright-config](https://github.com/microsoft/pyright/blob/main/docs/configuration.md#type-check-diagnostics-settings) directives with the format `# pyright: <config_name>=<value>`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - (Optional) Add a comment `## End of test code ##`. Several [pyright-config](https://github.com/microsoft/pyright/blob/main/docs/configuration.md#type-check-diagnostics-settings) with the format `# pyright: <config_name>=<value>` | |
| - (Optional) Add a comment `## End of test code ##`, followed by [pyright-config](https://github.com/microsoft/pyright/blob/main/docs/configuration.md#type-check-diagnostics-settings) directives with the format `# pyright: <config_name>=<value>` |
🤖 Prompt for AI Agents
In @docs/Contribute.md at line 45, The sentence fragment should be rewritten so
it reads as a complete instruction: change the line that currently reads about
the optional comment marker and pyright config to something like: "Optionally
add a comment `## End of test code ##`. You may also include pyright
configuration options using the format `# pyright: <config_name>=<value>`."
Update the wording around the `## End of test code ##` marker and the `#
pyright: <config_name>=<value>` example so the two clauses form complete,
grammatical sentences.
| def get_test_code(path: Path): | ||
| TEST_SPLITTER = "\n## End of test code ##\n" | ||
| level, challenge_name = path.parent.name.split("-", maxsplit=1) | ||
|
|
||
| with solution_file.open() as f: | ||
| challenge_code = f.read() | ||
| challenge = Challenge( | ||
| name=challenge_name, level=Level(level), code=challenge_code | ||
| ) | ||
|
|
||
| return challenge.test_code.partition(TEST_SPLITTER)[0] | ||
|
|
||
| solution_test = get_test_code(solution_file) | ||
| question_test = get_test_code(solution_file.parent / "question.py") |
There was a problem hiding this comment.
Critical bug: get_test_code always reads from solution_file regardless of the path argument.
Line 15 opens solution_file instead of the path parameter. This means both solution_test and question_test are derived from the same file, causing the test to always pass trivially without actually verifying that solution and question test codes are identical.
🐛 Proposed fix
def get_test_code(path: Path):
TEST_SPLITTER = "\n## End of test code ##\n"
level, challenge_name = path.parent.name.split("-", maxsplit=1)
- with solution_file.open() as f:
+ with path.open() as f:
challenge_code = f.read()
challenge = Challenge(
name=challenge_name, level=Level(level), code=challenge_code
)
return challenge.test_code.partition(TEST_SPLITTER)[0]🤖 Prompt for AI Agents
In @tests/test_identical.py around lines 11 - 24, get_test_code currently
ignores its path arg and always reads solution_file, so both solution_test and
question_test come from the same file; open and read the provided path parameter
instead of solution_file inside get_test_code (fix the file open call used when
building challenge_code and test extraction), keeping the rest of the function
logic the same so solution_test and question_test are derived from their
respective paths.
| # Error for pyright-config will not fail the challenge | ||
| passed = True | ||
| for error_line in error_lines: | ||
| if error_line.startswith("[pyright-config]"): | ||
| continue | ||
| passed = False | ||
|
|
||
| error_lines.append(f"\nFound {len(error_lines)} errors") |
There was a problem hiding this comment.
Minor: Error count includes non-failing pyright-config errors, which may confuse users.
A user might see "Found 1 errors" but still pass the challenge if the only error is from pyright-config. Consider either:
- Excluding pyright-config errors from the count, or
- Clarifying in the message (e.g., "Found 1 errors (0 blocking)")
Also, minor grammar nit: "Found 1 errors" should be "Found 1 error" for singular.
💡 Suggested improvement
- error_lines.append(f"\nFound {len(error_lines)} errors")
+ blocking_count = sum(1 for e in error_lines if not e.startswith("[pyright-config]"))
+ error_lines.append(f"\nFound {blocking_count} error{'s' if blocking_count != 1 else ''}")🤖 Prompt for AI Agents
In @views/challenge.py around lines 261 - 268, The error count currently
includes non-blocking pyright-config lines and the singular/plural grammar is
wrong; filter out lines starting with "[pyright-config]" into a blocking_errors
list and use that to determine passed (set passed = True when
len(blocking_errors) == 0) and to build the summary message, e.g., append
f"\nFound {n} error" vs "errors" based on n where n = len(blocking_errors), or
include both totals like "Found X errors (Y blocking)"; update references to
error_lines and the append call to use the new counts.
There was a problem hiding this comment.
@laike9m Understood! I'll defer to your judgment on this. Thanks for the feedback.
There was a problem hiding this comment.
The suggested solution seems complicated. Can you work on a better solution?
There was a problem hiding this comment.
@laike9m Sure! Here's a simpler approach that adds a counter to the existing loop where you're already checking for pyright-config errors:
# Error for pyright-config will not fail the challenge
passed = True
blocking_errors = 0
for error_line in error_lines:
if error_line.startswith("[pyright-config]"):
continue
blocking_errors += 1
passed = False
error_word = "error" if blocking_errors == 1 else "errors"
error_lines.append(f"\nFound {blocking_errors} {error_word}")This reuses the loop you already have, so there's no extra iteration. It counts only the blocking errors and fixes the singular/plural grammar issue. Does this work better for you?
2 participants
New feature
support test_code to add pyright-config as below.
Bug fix
disable user to modify pyright-config.
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.